Implement MariaDB support#8
Conversation
…tests Co-authored-by: alexandru <11753+alexandru@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds MariaDB as a supported JDBC backend for DelayedQueueJDBC, including vendor-specific SQL (locking + concurrent polling), migrations, exception classification, and a Testcontainers-backed test suite.
Changes:
- Introduces
JdbcDriver.MariaDBand wires it into adapter creation, migrations, and exception filters. - Adds a MariaDB-specific
SQLVendorAdapterimplementation and DDL migrations for the queue table. - Adds MariaDB-focused unit tests (SQL generation, exception filters) plus contract/advanced/concurrency suites via Testcontainers.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Adds MariaDB JDBC driver + MariaDB Testcontainers module to the version catalog. |
| delayedqueue-jvm/build.gradle.kts | Includes MariaDB JDBC/Testcontainers dependencies for tests. |
| delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/JdbcDriver.kt | Adds the MariaDB driver enum entry + driver class name. |
| delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/DelayedQueueJDBC.kt | Adds MariaDB migrations; broadens batch-offer fallback to include transient failures. |
| delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/SQLVendorAdapter.kt | Adds MariaDBAdapter implementing INSERT IGNORE and FOR UPDATE SKIP LOCKED patterns. |
| delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/MariaDBMigrations.kt | New MariaDB table/index DDL (InnoDB, utf8mb4, MEDIUMBLOB, composite PK + unique id). |
| delayedqueue-jvm/src/main/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/SqlExceptionFilters.kt | Adds MariaDB-specific exception filters and maps driver -> filters. |
| delayedqueue-jvm/src/test/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/SqlExceptionFiltersTest.kt | Adds MariaDB filter tests and verifies filtersForDriver(MariaDB). |
| delayedqueue-jvm/src/test/kotlin/org/funfix/delayedqueue/jvm/internals/jdbc/MariaDBAdapterSqlTests.kt | New unit tests asserting MariaDB-specific SQL generation. |
| delayedqueue-jvm/src/test/java/org/funfix/delayedqueue/api/MariaDbTestContainer.java | New singleton Testcontainers helper for MariaDB integration tests. |
| delayedqueue-jvm/src/test/java/org/funfix/delayedqueue/api/DelayedQueueJDBCMariaDbTest.java | Runs the contract test suite against MariaDB Testcontainers. |
| delayedqueue-jvm/src/test/java/org/funfix/delayedqueue/api/DelayedQueueJDBCMariaDbConcurrencyTest.java | Runs concurrency tests against MariaDB Testcontainers. |
| delayedqueue-jvm/src/test/java/org/funfix/delayedqueue/api/DelayedQueueJDBCMariaDbAdvancedTest.java | Runs advanced tests against MariaDB Testcontainers. |
| delayedqueue-jvm/src/test/java/org/funfix/delayedqueue/api/JdbcDriverTest.java | Extends Java switch exhaustiveness tests to include MariaDB. |
| delayedqueue-jvm/api/delayedqueue-jvm.api | Updates the API dump to include the new JdbcDriver.MariaDB entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| override fun insertOneRow(connection: Connection, row: DBTableRow): Boolean { | ||
| val sql = | ||
| """ | ||
| INSERT IGNORE INTO $tableName | ||
| (pKey, pKind, payload, scheduledAt, scheduledAtInitially, createdAt) | ||
| VALUES (?, ?, ?, ?, ?, ?) | ||
| """ | ||
|
|
||
| return connection.prepareStatement(sql).use { stmt -> | ||
| stmt.setString(1, row.pKey) | ||
| stmt.setString(2, row.pKind) | ||
| stmt.setBytes(3, row.payload) | ||
| stmt.setEpochMillis(4, row.scheduledAt) | ||
| stmt.setEpochMillis(5, row.scheduledAtInitially) | ||
| stmt.setEpochMillis(6, row.createdAt) | ||
| stmt.executeUpdate() > 0 | ||
| } |
There was a problem hiding this comment.
MariaDBAdapter.insertOneRow uses INSERT IGNORE, which suppresses many non-duplicate insert errors in MariaDB/MySQL (e.g., truncation, invalid data) by converting them to warnings and returning 0 rows affected. That can cause silent message loss/corruption while being treated as “key already exists”. Consider using INSERT ... ON DUPLICATE KEY UPDATE (no-op update) or a plain INSERT with duplicate-key detection so unexpected insert failures still throw.
| when { | ||
| CommonSqlFilters.transactionTransient.matches(e) -> true | ||
| e is SQLException && e.errorCode == 1213 -> true // Deadlock | ||
| e is SQLException && e.errorCode == 1205 -> true // Lock wait timeout | ||
| e is SQLException && e.errorCode == 1020 -> | ||
| true // Record changed since last read | ||
| else -> { | ||
| val cause = e.cause | ||
| cause != null && cause !== e && matches(cause) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
MariaDBFilters.transientFailure only walks Throwable.cause, but JDBC batch failures commonly surface vendor SQLExceptions via SQLException.getNextException() / BatchUpdateException.getNextException() rather than cause. This means error codes like 1020/1213/1205 may not be detected in real batch scenarios (contradicting the PR description). Consider traversing the nextException chain for SQLException (and/or handling BatchUpdateException) in addition to cause.
| when { | |
| CommonSqlFilters.transactionTransient.matches(e) -> true | |
| e is SQLException && e.errorCode == 1213 -> true // Deadlock | |
| e is SQLException && e.errorCode == 1205 -> true // Lock wait timeout | |
| e is SQLException && e.errorCode == 1020 -> | |
| true // Record changed since last read | |
| else -> { | |
| val cause = e.cause | |
| cause != null && cause !== e && matches(cause) | |
| } | |
| } | |
| } | |
| matchesTransientRecursively(e) | |
| } | |
| /** | |
| * Recursively checks whether the given throwable or any of its nested causes / SQL nextExceptions | |
| * represent a transient MariaDB failure. | |
| */ | |
| private fun matchesTransientRecursively(throwable: Throwable?): Boolean { | |
| if (throwable == null) return false | |
| // Delegate to common transaction transient checks first. | |
| if (CommonSqlFilters.transactionTransient.matches(throwable)) return true | |
| if (throwable is SQLException) { | |
| // MariaDB-specific transient error codes. | |
| when (throwable.errorCode) { | |
| 1213, // Deadlock | |
| 1205, // Lock wait timeout | |
| 1020 -> // Record changed since last read | |
| return true | |
| } | |
| // Walk the nextException chain commonly used in batch failures. | |
| var next: SQLException? = throwable.nextException | |
| while (next != null && next !== throwable) { | |
| if (matchesTransientRecursively(next)) return true | |
| next = next.nextException | |
| } | |
| } | |
| // Fallback: walk the cause chain, as originally implemented. | |
| val cause = throwable.cause | |
| return cause != null && cause !== throwable && matchesTransientRecursively(cause) | |
| } |
| @Test | ||
| fun `transientFailure should match record changed in cause chain`() { | ||
| val cause = SQLException("Record has changed since last read", "HY000", 1020) | ||
| val wrapper = RuntimeException("batch failed", cause) | ||
| assertTrue(MariaDBFilters.transientFailure.matches(wrapper)) | ||
| } |
There was a problem hiding this comment.
MariaDBFilters tests cover matching via cause, but don’t cover the common JDBC pattern where BatchUpdateException/SQLException store the vendor error in the nextException chain (not cause). Adding a test for BatchUpdateException with nextException set to a SQLException(errorCode=1020/1213/1205) would guard against regressions and validate the intended behavior.
| container = | ||
| new MariaDBContainer<>(IMAGE) | ||
| .withDatabaseName("testdb") | ||
| .withUsername("test") | ||
| .withPassword("test"); | ||
| container.start(); |
There was a problem hiding this comment.
Potential race condition. This assignment to container is visible to other threads before the subsequent statements are executed.
| container = | |
| new MariaDBContainer<>(IMAGE) | |
| .withDatabaseName("testdb") | |
| .withUsername("test") | |
| .withPassword("test"); | |
| container.start(); | |
| MariaDBContainer<?> local = | |
| new MariaDBContainer<>(IMAGE) | |
| .withDatabaseName("testdb") | |
| .withUsername("test") | |
| .withPassword("test"); | |
| local.start(); | |
| container = local; |
|
@codex please review. NOTE: this PR contains some refactoring unrelated to MariaDB, please focus on the MariaDB integration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdad29316a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| INSERT IGNORE INTO $tableName | ||
| (pKey, pKind, payload, scheduledAt, scheduledAtInitially, createdAt) | ||
| VALUES (?, ?, ?, ?, ?, ?) |
There was a problem hiding this comment.
Avoid INSERT IGNORE to prevent silent data loss
Using INSERT IGNORE means MariaDB will downgrade many errors (e.g., data truncation or constraint violations unrelated to duplicates) to warnings and return 0/1 affected rows. In this code path that result is treated as “already exists” or “inserted”, so malformed or oversized payloads/keys can be silently dropped or corrupted instead of raising a failure for retries or surfacing to callers. This is a correctness issue that can surface when inputs exceed column limits or violate constraints, and it only happens because IGNORE suppresses those errors.
Useful? React with 👍 / 👎.
| lockUuid VARCHAR(36) NULL, | ||
| createdAt BIGINT NOT NULL, | ||
| PRIMARY KEY (pKey, pKind), | ||
| UNIQUE KEY ${tableName}__IdUniqueIndex (id) |
There was a problem hiding this comment.
Use a numeric primary key to avoid InnoDB index bloat
Defining the primary key as (pKey, pKind) makes InnoDB cluster rows on two large VARCHARs; every secondary index (including the hot (pKind, scheduledAt) index used for polling) will carry those PK columns. That inflates index size and slows scans/locking compared to using id as the PK with a separate unique index on (pKind, pKey), which is how the other backends are structured. This is a performance regression specifically on MariaDB under load.
Useful? React with 👍 / 👎.
Adds MariaDB as a supported database backend, using InnoDB-specific locking idioms for optimal concurrent performance.
Adapter (
MariaDBAdapter)INSERT IGNOREfor atomic conditional inserts (no exception on duplicate, unlike HSQLDB's try-catch)SELECT ... FOR UPDATE SKIP LOCKEDfor concurrent polling — consumers skip locked rows instead of blocking (analogous to MS-SQL'sREADPAST)UPDATE ... WHERE id IN (SELECT ... FOR UPDATE SKIP LOCKED)wrapped in derived table subquery (MariaDB parser requirement)Migrations
BIGINT AUTO_INCREMENTwithUNIQUE KEY(required for non-PK auto-increment), compositePRIMARY KEY (pKey, pKind),MEDIUMBLOBpayload,InnoDBengine,utf8mb4Exception filters (
MariaDBFilters)BatchUpdateExceptionBatch insert resilience
offerBatchImplnow also falls back to one-by-one on transient failures, not just duplicate keys — fixesBatchUpdateExceptionwith error 1020 during concurrent batch inserts. Benefits all drivers.Usage
Tests
MariaDBAdapterSqlTests.kt)mariadb:11.7)Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
mvnrepository.com/home/REDACTED/work/_temp/ghcca-node/node/bin/node /home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.